Developer by dmitromurashov-lgtm · Pull Request #6280 · mate-academy/layout_catalog

dmitromurashov-lgtm · 2026-06-23T15:51:55Z

mateacademy-ai-mentor

Review Summary

I am requesting changes because several core task requirements and checklist items are still not met, and these are considered blocking. Most importantly, the first nav link does not use the required is-active class, the header and stars blocks are not fully rewritten using BEM (.logo, .nav-list, .star instead of elements like .header__logo, .stars__star), and the cards container/grid in <main> does not implement the 1/2/3/4-column layout at the specified breakpoints with fixed 50px 40px paddings ([CHECKLIST ITEM #1], [CHECKLIST ITEM #3], [CHECKLIST ITEM #4]). In addition, the card title hover behavior is implemented as .product-card__title:hover instead of .product-card:hover .product-card__title, and hover transitions for nav links and the button are missing or inconsistent with the required smooth 300ms animation ([CHECKLIST ITEM #5]). You also repeatedly hard-code special colors like #060b35, #34568b, and #f3f3f3 instead of putting them into SCSS variables in variables.scss and reusing them ([CHECKLIST ITEM #2]).

To fix this, 1) update the first nav link in src/index.html to include the literal is-active class and ensure the SCSS targets a.is-active for styling; 2) refactor header and stars classes to proper BEM elements (e.g. .header__logo, .header__nav-link, .stars__star) and adjust the HTML accordingly; 3) move the catalog grid and padding rules to <main> (or a BEM catalog block) and implement CSS Grid with 1 column <488px, 2 columns ≥488px, 3 columns ≥768px, and 4 columns ≥1024px, always using padding: 50px 40px;. Then, 4) change the card hover selector to .product-card:hover .product-card__title { color: $color-card-title-hover; transition: color 300ms; } and remove the direct &__title:hover rule, and 5) add transition: color 300ms; on .nav-link and transition: background-color 300ms, color 300ms; on the button so all hover changes are smooth. Finally, 6) define variables for the repeated colors (#060b35, #34568b, #f3f3f3, #fff) in variables.scss and replace hard-coded occurrences in header.scss, cards.scss, and index.scss. Your overall structure and card/stars blocks are close, so once these required adjustments are in place, the solution should fully satisfy the task and checklist; keep going, you’re nearly there.

✨ Please rate this review in the Platform's notification center¹. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button² in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag³.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

mateacademy-ai-mentor · 2026-06-23T15:53:42Z

+          <li class="nav-items">
+            <a
+              href="#"
+              class="nav-link nav-link--active"


This violates the description requirement: "add the class is-active to the first link (Apple) in the navigation". Here the first link uses nav-link nav-link--active, which is a different modifier name. Rename the modifier in HTML to match the required is-active class (and adjust the SCSS selector accordingly) so tests can find it.

mateacademy-ai-mentor · 2026-06-23T15:53:42Z

+    <header class="header">
+      <a href="#">
+        <img
+          src="images/logo.png"
+          alt="Moyo logo"
+          class="logo"
+        />
+      </a>
+      <nav class="nav">
+        <ul class="nav-list">
+          <li class="nav-items">
+            <a


This violates [CHECKLIST ITEM #1]: "All components follow BEM and use SCSS". The header/navigation uses generic classes like logo, nav-list, nav-items, nav-link instead of BEM block/element names (e.g. header__logo, header__nav, header__nav-list, header__nav-item, etc.). Consider refactoring the header markup and corresponding SCSS to follow BEM naming for the Header block.

mateacademy-ai-mentor · 2026-06-23T15:53:43Z

+        <img
+          src="images/logo.png"
+          alt="Moyo logo"
+          class="logo"
+        />
+      </a>
+      <nav class="nav">


This violates the description requirement: "nav links color is not black anymore (nav links should have #060b35 color" and [CHECKLIST ITEM #2] "Repeated sizes and special colors are put to variables". The color #060b35 is hardcoded both here and in cards.scss. Since it’s a special color used in multiple places, consider extracting it into a SCSS variable (e.g. $color-nav) and reusing that variable.

mateacademy-ai-mentor · 2026-06-23T15:53:43Z

+  &:hover {
+    color: #34568b;
+    transform: scale(1.2);


The first nav link uses class="nav-link nav-link--active", but the description requires: add the class is-active to the first link (Apple) in the navigation. You should add the literal is-active class here (you may keep the BEM modifier for styling if you want).

mateacademy-ai-mentor · 2026-06-23T15:53:43Z

+  &__code {
+    font-family: $font-family;
+    font-weight: $font-weight-regular;
+    font-size: $font-size;


Task requires hover behavior to be smooth everywhere and for nav links: change navigation link text color to #00acdc. You have the color change, but there is no transition on .nav-link, so the change is instant. Add a transition: color 300ms; (300ms as per specs) to satisfy checklist item "All changes on :hover are smooth".

mateacademy-ai-mentor · 2026-06-23T15:53:43Z

+    border-radius: 5px;
+    width: 166px;


The container inside <main> is named .container, but the task expects a semantic catalog structure and BEM components (Header, Stars, Card). Consider renaming this to a catalog block (e.g. .catalog) and using BEM elements for the cards wrapper to align with the requirement that all components follow BEM.

mateacademy-ai-mentor · 2026-06-23T15:53:43Z

+    height: 40px;
+    text-decoration: none;
+    color: #fff;


Good that you added data-qa="card" to the first card. However, for BEM/consistency, the block name is .product-card while the task refers to a Card block. That’s fine naming-wise, but make sure your main catalog container and header follow BEM too; right now only the card and stars blocks do.

mateacademy-ai-mentor · 2026-06-23T15:53:43Z

+  &__title {
+    font-family: $font-family;
+    font-weight: $font-weight-medium;
+    font-size: $font-size-biger;
+    margin-top: 40px;
+    margin-left: 16px;
+    line-height: 18px;
+    width: 166px;
+    height: 36px;
+    color: #060b35;
+    transition: color 0.3s ease;
+
+    &:hover {
+      color: #34568b;
+    }


The hover color change for the card title is implemented as .product-card__title:hover, but the description explicitly says: change the card title text color to #34568b when the card is hovered (.card:hover .card__title). This means the whole card hover should trigger the title color change, not hovering the title itself. Move this rule to something like .product-card:hover .product-card__title { color: #34568b; } and remove the &__title:hover block.

mateacademy-ai-mentor · 2026-06-23T15:53:43Z

+  &:hover {
+    color: #34568b;
+    transform: scale(1.2);
+  }


Here the entire .product-card changes text color on hover via &:hover { color: #34568b; }, which is not part of the spec and may conflict with more targeted styling. The requirement is only to change the title color on card hover. Consider limiting hover styling to transform: scale(1.2); plus a separate :hover rule for the title as described in the task.

mateacademy-ai-mentor · 2026-06-23T15:53:43Z

+  &__button {
+    border: 1px solid $color-primary;
+    display: flex;
+    justify-content: center;
+    align-items: center;
+    box-sizing: border-box;
+    background-color: $color-primary;
+    border-radius: 5px;
+    width: 166px;
+    height: 40px;
+    text-decoration: none;
+    color: #fff;
+    font-family: $font-family;
+    font-size: 14px;
+    font-weight: $font-weight-bold;
+    margin-top: 16px;
+    margin-left: 16px;
+    margin-bottom: 16px;
+    line-height: 16px;
+
+    &:hover {
+      background-color: #fff;
+      color: $color-primary;
+    }


For checklist item "All changes on :hover are smooth" and the description’s 300ms requirement, the button does not have any transition defined. Add a transition on background and color (e.g. transition: background-color 300ms, color 300ms;) so the hover to white background and primary text color animates smoothly.

mateacademy-ai-mentor · 2026-06-23T15:53:47Z

+  &__title {
+    font-family: $font-family;
+    font-weight: $font-weight-medium;
+    font-size: $font-size-biger;
+    margin-top: 40px;
+    margin-left: 16px;
+    line-height: 18px;
+    width: 166px;
+    height: 36px;
+    color: #060b35;


#060b35 is used here but is also used in several other places (e.g. in header.scss and other card text). Checklist item states "Repeated sizes and special colors are put to variables"; consider adding a variable for this color in variables.scss and using it here.

mateacademy-ai-mentor · 2026-06-23T15:53:47Z

+    background-image: url(..//images/Rectangle\ 4.svg);
+    width: 160px;
+    height: 134px;
+    line-height: 18px;
+    background-size: contain;
+    margin: 32px auto 0;
+  }
+
+  &__title {
+    font-family: $font-family;
+    font-weight: $font-weight-medium;
+    font-size: $font-size-biger;
+    margin-top: 40px;
+    margin-left: 16px;
+    line-height: 18px;
+    width: 166px;
+    height: 36px;
+    color: #060b35;
+    transition: color 0.3s ease;
+
+    &:hover {
+      color: #34568b;
+    }
+  }
+
+  &:hover {


Header block is not BEM-compliant: .logo, .nav-list, .nav-items, .nav-link are free-standing classes, not elements of a header block. The checklist requires "All components follow BEM and use SCSS" and the description asks to rewrite the Header block using BEM. Consider renaming to something like .header__logo, .header__nav, .header__nav-list, .header__nav-item, .header__nav-link.

mateacademy-ai-mentor · 2026-06-23T15:53:47Z

+    height: 36px;
+    color: #060b35;
+    transition: color 0.3s ease;
+
+    &:hover {
+      color: #34568b;
+    }


#060b35 is hard-coded here and also appears in cards.scss. Per checklist item "Repeated sizes and special colors are put to variables", you should move this color to variables.scss and reference the variable instead.

mateacademy-ai-mentor · 2026-06-23T15:53:47Z

+
+.nav-items {
+  margin-right: 20px;
+


The active navigation item should use the is-active class as required by the description ("add the class is-active to the first link (Apple)"), but you currently use nav-link nav-link--active. Update the class to is-active or adjust your CSS to match the required class name.

mateacademy-ai-mentor · 2026-06-23T15:53:47Z

+  padding: 0;
+  list-style-type: none;
+  color: #060b35;


Hover styling for the card title is specified as .card:hover .card__title in the description, but here you only change the title color on &__title:hover. You should instead target the title when the whole .product-card is hovered, per requirement.

mateacademy-ai-mentor · 2026-06-23T15:53:47Z

+.nav-items {
+  margin-right: 20px;
+


The card hover is supposed to change only the title color according to .card:hover .card__title, but this rule sets color: #34568b on the entire .product-card, potentially affecting other text. Adjust the selector so only the title gets the color change, in line with the requirement.

mateacademy-ai-mentor · 2026-06-23T15:53:47Z

+  &:hover {
+    color: var(--blue);


Hover color changes for nav links must be smooth over 300ms (checklist: "All changes on :hover are smooth"), but .nav-link defines a hover color without any transition. Consider adding a transition on color to make the hover animation smooth.

mateacademy-ai-mentor · 2026-06-23T15:53:47Z

+.header {
+  display: flex;
+  align-items: center;
+  justify-content: space-between;
+  padding: 0 50px;
+  background-color: #fff;
+  box-sizing: border-box;
+}
+
+.logo {
+  display: block;
+  width: 40px;
+  height: 40px;
+  margin: 0;
+}
+
+.nav-list {
+  display: flex;
+  justify-content: flex-end;
+  margin: 0;
+  padding: 0;
+  list-style-type: none;
+  color: #060b35;
+}
+
+.nav-items {
+  margin-right: 20px;
+
+  &:last-child {
+    margin-right: 0;
+  }
+}
+
+.nav-link {
+  display: inline-flex;
+  height: 60px;
+  align-items: center;
+  justify-content: center;
+  text-transform: uppercase;
+  text-decoration: none;
+  overflow: visible;
+  color: #060b35;
+  font-family: Roboto, sans-serif;
+  font-weight: 500;
+  font-size: 12px;
+  line-height: 100%;
+
+  &--active {
+    color: $color-primary;
+    position: relative;
+
+    &::after {
+      content: '';
+      position: absolute;
+      left: 0;
+      right: 0;
+
+      height: 4px;
+      background: var(--blue);
+      bottom: -8px;
+      bottom: 0;
+      border-radius: 8px;
+    }
+  }
+
+  &:hover {
+    color: var(--blue);
+  }
+}


Header and nav-related classes (.logo, .nav-list, .nav-items, .nav-link) are declared as separate blocks instead of elements of .header (e.g., .header__logo, .header__nav-list), which breaks the BEM requirement ("All components follow BEM"). Refactor these to follow BEM naming.

mateacademy-ai-mentor · 2026-06-23T15:53:47Z

  margin: 0;
 }
+
+@media screen and (min-width: 1024px) {


This violates checklist item #4: "Cards are shown in 1, 2, 3, or 4 columns based on screen resolution". Currently the .container grid is only defined for min-width: 1024px (and 1200px) with 4 columns; there are no styles implementing 1 column for smaller screens, 2 columns starting at 488px, and 3 columns starting from 768px.

mateacademy-ai-mentor · 2026-06-23T15:53:47Z

+  .container {
+    display: grid;
+    grid-template-columns: repeat(4, 200px);
+    padding: 50px 40px;
+    gap: 46px 48px;
+  }
+}
+
+@media screen and (min-width: 1200px) {
+  .container {
+    display: grid;
+    grid-template-columns: repeat(4, 200px);
+    padding: 50px 128px;
+    gap: 46px 48px;
+  }


This violates checklist item #3: "Grid is used for the columns" across all specified resolutions. At the moment, .container uses grid only from 1024px upward. You need to use grid at all breakpoints with the required column counts (1 / 2 / 3 / 4).

mateacademy-ai-mentor · 2026-06-23T15:53:51Z

 }
+
+@media screen and (min-width: 1024px) {
+  .container {


This violates checklist item #2: "Repeated sizes and special colors are put to variables". Colors like #060b35 and #f3f3f3 are used as raw values instead of SCSS variables, while you already have a variables file for other colors.

mateacademy-ai-mentor · 2026-06-23T15:53:51Z

+.stars {
+  display: flex;
+
+  .star {
+    background-image: url(../images/star.svg);
+    width: 16px;
+    height: 16px;
+    background-position: center;
+    background-repeat: no-repeat;
+
+    &:not(:last-child) {
+      margin-right: 4px;
+    }
+  }


This violates [CHECKLIST ITEM #1]: "All components follow BEM and use SCSS" for the Stars block. Inside .stars you style a nested .star class instead of a BEM element like .stars__star. Similarly, the modifiers &--1–&--5 are fine as BEM modifiers, but they still target .star instead of a BEM element. To fully comply, rename .star to .stars__star in both HTML and SCSS and adjust these selectors accordingly.

mateacademy-ai-mentor · 2026-06-23T15:53:51Z

+.stars {
+  display: flex;
+
+  .star {
+    background-image: url(../images/star.svg);
+    width: 16px;
+    height: 16px;
+    background-position: center;


You’ve defined SCSS variables for font family, weights, sizes, and two colors, which is good. However, [CHECKLIST ITEM #2] requires "Repeated sizes and special colors are put to variables", and colors like #060b35, #34568b, and #f3f3f3 (used in other SCSS files) are not yet represented here. Consider adding variables for those repeated colors and using them in header.scss and cards.scss to fully satisfy the checklist.

mateacademy-ai-mentor · 2026-06-23T15:53:51Z

+$font-family: Roboto, sans-serif;
+$font-weight-regular: 400;
+$font-weight-medium: 500;
+$font-weight-bold: 700;
+$font-size: 10px;
+$font-size-biger: 12px;
+$color-primary: #00acdc;
+$color-secondary: #616070;


Checklist item #2 requires "Repeated sizes and special colors are put to variables". While you have variables for some colors and font sizes, other special colors used elsewhere (e.g. #060b35, #34568b, #f3f3f3) are not defined here. Consider adding variables for all repeated/specified colors so they can be reused consistently across SCSS files.

Anton-Kuchmasov

Good job!

UI diffs are not crucial, so they can't be blocker.

Go ahead!

add task solution

b76bc36

mateacademy-ai-mentor suggested changes Jun 23, 2026

View reviewed changes

mateacademy-ai-mentor reviewed Jun 23, 2026

View reviewed changes

Anton-Kuchmasov approved these changes Jun 23, 2026

View reviewed changes

Uh oh!

Conversation

dmitromurashov-lgtm commented Jun 23, 2026

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Anton-Kuchmasov left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants